-
Notifications
You must be signed in to change notification settings - Fork 4
Unified Compressed Pointer #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
37218e1 to
b5dcfea
Compare
igchor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @guptask)
cachelib/allocator/memory/CompressedPtr.h line 49 at r1 (raw file):
// the following are for pointer compression for the memory allocator. We // compress pointers by storing the tier index, slab index and alloc index // of the allocation inside the slab. With slab worth kNumSlabBits (22 bits)
why this description is repeated?
cachelib/allocator/memory/CompressedPtr.h line 82 at r1 (raw file):
// maximum adressable memory for pointer compression to work. static constexpr size_t getMaxAddressableSize() noexcept { return static_cast<size_t>(1) << (kNumSlabIdxBits + Slab::kNumSlabBits + 1);
why +1? we have the same total addressable memory in total - half for each tier, right?
Can you add some test for this?
cachelib/allocator/memory/CompressedPtr.h line 139 at r1 (raw file):
return (slabIdx << kNumAllocIdxBits) + allocIdx; } XDCHECK_LT(slabIdx, (1u << kNumSlabIdxBits) - 1);
is this assert correct? with tier ID bit, we have less bits for slab, right?
You could also add ASSERT for tid I guess (< 2).
cachelib/allocator/memory/CompressedPtr.h line 214 at r1 (raw file):
} bool isMultiTiered = allocators_.size() > 1 ? true : false;
you can drop ? true : false part
cachelib/allocator/memory/CompressedPtr.h line 216 at r1 (raw file):
bool isMultiTiered = allocators_.size() > 1 ? true : false; auto cptr = allocators_[tid]->compress(uncompressed, isMultiTiered); if (allocators_.size() > 1) { // config has multiple tiers
why not just use isMultiTIered?
cachelib/allocator/memory/CompressedPtr.h line 226 at r1 (raw file):
return nullptr; } bool isMultiTiered = allocators_.size() > 1 ? true : false;
same here
cachelib/allocator/tests/AllocatorMemoryTiersTest.h line 139 at r1 (raw file):
slabStats = allocator->getAllocationClassStats(0,0,cid); } ASSERT_GE(slabStats.approxFreePercent,9.5);
why this is removed?
|
Previously, igchor (Igor Chorążewicz) wrote…
fixed |
|
Previously, igchor (Igor Chorążewicz) wrote…
I changed kNumTierIdxOffset to 31 from 32. |
|
Previously, guptask (Sounak Gupta) wrote…
kNumSlabIdxBits = kNumTierIdxOffset (31) - kNumAllocIdxBits (16) |
|
Previously, igchor (Igor Chorążewicz) wrote…
In single tier mode, CompressedPtr doesn't reserve the 32nd bit for tier id. In this PR, kNumTierIdxOffset was changed from 32 to 31. So kNumSlabIdxBits = kNumTierIdxOffset - kNumAllocIdxBits = 31-16 = 15. |
|
Previously, igchor (Igor Chorążewicz) wrote…
moving to a separate commit since this fix is not directly related to CompressedPtr PR |
|
Previously, igchor (Igor Chorążewicz) wrote…
done |
|
Previously, igchor (Igor Chorążewicz) wrote…
done |
|
Previously, igchor (Igor Chorążewicz) wrote…
done |
…ng in single tier mode and use 31 bits for addressing in multi-tier mode
…e to 9.5 but not guaranteed to be >= 9.5
igchor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@guptask can you run a hit_ratio leader and follower benchmark to see if there is no regression? I don't expect any but it would be good to make sure everything works as expected.
Reviewable status: 9 of 10 files reviewed, all discussions resolved (waiting on @guptask)
|
done. regression not observed. |
byrnedj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @guptask)
Fix eviction flow and removeCb calls
added ability for compressed pointer to use full 32 bits for addressing in single tier mode and use 31 bits for addressing in multi-tier mode
This change is